Skip to content

rename and update mergo module#4378

Merged
thaJeztah merged 1 commit intodocker:masterfrom
acim:master
Jun 28, 2023
Merged

rename and update mergo module#4378
thaJeztah merged 1 commit intodocker:masterfrom
acim:master

Conversation

@acim
Copy link
Copy Markdown
Contributor

@acim acim commented Jun 26, 2023 •
edited
Loading

- What I did

I renamed module github.com/imdario/mergo to dario.cat/mergo and updated to the latest version. This fixes #4377.

- How I did it

I renamed the import in merge.go and merge_test.go and ran make vendor.

- How to verify it

Run tests.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@acim acim changed the title rename and update mergo module #4377 rename and update mergo module Jun 26, 2023
@acim acim changed the title #4377 rename and update mergo module rename and update mergo module Jun 26, 2023
@acim
rename and update mergo module
b32264a 
Signed-off-by: Boban Acimovic <boban.acimovic@gmail.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 27, 2023

Codecov Report

Merging #4378 (b32264a) into master (605942c) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #4378   +/-   ##
=======================================
  Coverage   59.29%   59.29%           
=======================================
  Files         288      288           
  Lines       24769    24769           
=======================================
  Hits        14687    14687           
  Misses       9198     9198           
  Partials      884      884

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 27, 2023

Thanks! I'm wondering what motivated the move to a vanity URL; I hope there's good hosting behind it, as I've had bad experience with vanity URLs not always being very reliable

@acim
Copy link
Copy Markdown
Contributor Author

acim commented Jun 27, 2023 •
edited
Loading

Well, I don't know, I am not the author/maintainer of that module. Since you are using vendoring it is probably not that unsafe.

neersighted
neersighted approved these changes Jun 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@neersighted neersighted left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not quite inherently safe since we do (among other things) check that vendoring is correct in CI, which requires network access to the module source code. That being said, we should allow GOPROXY for that -- in the past we had it disabled for $REASONS, but it is designed to solve this problem (validation/fetching of code already in your go.sum).

Basically, any issues we have are largely self-inflicted and we can work them out. LGTM!

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jun 28, 2023

Yeah, it's definitely not a blocker, just bad experience with vanity URLs going AWOL. While GOPROXY is useful for temporary downtime of those, it's not a permanent fix; I've had situations where vanity URLs actually went down (permanently), in which case it's a ticking time bomb until GOPROXY's cache expires. Of course using the upstream code repository (github.com) does not help for situations where project source is actually removed, but for other cases I have more trust in github.com's availability / SLA, than domains hosted by a single individual.

@thaJeztah thaJeztah added this to the 25.0.0 milestone Jun 28, 2023
thaJeztah
thaJeztah approved these changes Jun 28, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 697bd4b into docker:master Jun 28, 2023
@darccio
Copy link
Copy Markdown

darccio commented Aug 22, 2023

@thaJeztah Allow me to indicate that the hosting is CloudFlare, and I plan to keep the domain. It shouldn't be an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@neersighted neersighted neersighted approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

25.0.0

Development

Successfully merging this pull request may close these issues.

github.com/imdario/mergo module renamed to dario.cat/mergo

5 participants

@acim @codecov-commenter @thaJeztah @darccio @neersighted